-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
discovery: add rails service name detector #30091
base: main
Are you sure you want to change the base?
discovery: add rails service name detector #30091
Conversation
0254171
to
3eee457
Compare
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=46912584 --os-family=ubuntu Note: This applies to commit 6b70228 |
Regression Detector |
c2e1b5f
to
535027d
Compare
535027d
to
eb3ca59
Compare
pkg/collector/corechecks/servicediscovery/usm/testdata/root/testdata/application.rb
Outdated
Show resolved
Hide resolved
pkg/collector/corechecks/servicediscovery/usm/testdata/root/testdata/application_invalid.rb
Outdated
Show resolved
Hide resolved
reader, err := SizeVerifiedReader(file) | ||
if err != nil { | ||
log.Debugf("skipping application.rb (%q): %v", filename, err) | ||
return "", true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will have a service without name, it does not make sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I totally agree with your comment, I do not think it should be the role of the rails detector (or any detectors) to handle that case.
This is also an issue for other detectors (e.g the Gunicorn one) we have and, IMO, should be handled in ExtractServiceMetadata
to call a fallback mechanism (simpleDetector?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be the responsibility of the caller, as long as we guarantee when we zoom out that we won't have services without names, I'm fine with any implementation
bytes, err := io.ReadAll(reader) | ||
if err != nil { | ||
log.Debugf("unable to read application.rb (%q): %v", filename, err) | ||
return "", true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snake := matchFirstCap.ReplaceAllString(pascalCasedWord, "${1}_${2}") | ||
snake = matchAllCap.ReplaceAllString(snake, "${1}_${2}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operations on strings are expensive, especially when regex is involved
can't we make a single call to ReplaceAllString?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked a bit into this, and AFAICT, doing it in one call to ReplaceAllString
would require lookahead (to find the next word first upper case letter, without actually matching it), which Go's regexp does not support.
So if we want to avoid doing that it looks like we need to make our own Pascal->Snake case converter.
{ | ||
name: "name is found", | ||
path: "./testdata/application.rb", | ||
expected: "rails_hello", | ||
}, | ||
{ | ||
name: "name not found", | ||
path: "./testdata/application_invalid.rb", | ||
expected: "", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as my other comment suggested, let's work on the test cases to gain full coverage of the function
What does this PR do?
Port the service naming logic for RubyOnRails application. from our injector. It should behave in the same way. This new detector is called when the command name extracted from the command line is
puma
(the command line of the application gets replaced when starting a RoR server).Motivation
USMON-1233
Describe how to test/QA your changes
Handled by E2E test.
Possible Drawbacks / Trade-offs
Additional Notes